Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(label): allow text to truncate #5364

Merged
merged 4 commits into from Mar 1, 2023
Merged

Conversation

mattnolting
Copy link
Contributor

@mattnolting mattnolting commented Feb 2, 2023

This PR: Closes #4795

Variables

Adds:

  • --pf-c-label--MaxWidth
  • --pf-c-label__text--m-truncate--MaxWidth

@patternfly-build
Copy link

patternfly-build commented Feb 2, 2023

@mattnolting mattnolting requested review from mcoker and srambach and removed request for mcoker February 8, 2023 16:18
@mattnolting mattnolting marked this pull request as ready for review February 8, 2023 16:19
@mattnolting mattnolting force-pushed the fix-label-4795 branch 2 times, most recently from 3d2cbff to c7b9857 Compare February 8, 2023 16:24
Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like there was a modifier added to truncate, but the issue was to truncate labels by default. Did that change? The labels that say the text should truncate do not truncate
Screenshot 2023-02-16 at 5 53 03 PM

src/patternfly/components/Label/label.hbs Outdated Show resolved Hide resolved
@@ -410,12 +415,15 @@
@include pf-text-overflow;

max-width: var(--pf-c-label__text--MaxWidth);

&.pf-m-truncate {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the issue no longer to truncate labels by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcoker This issue is still to truncate labels by default, but do so when the label runs out of room rather than at {x}ch width.

Screenshot 2023-02-22 at 3 38 23 PM

Perhaps the predefined width truncation pf-m-truncate modifier would be implemented by simply updating --pf-c-label__text--MaxWidth.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it. Should this class be on .pf-c-label tho?

src/patternfly/components/Label/label.hbs Outdated Show resolved Hide resolved
Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments, but otherwise LGTM. We do need to update the examples in some way since there is a label in each example block that says it will truncate. I don't know if we add .pf-m-truncate to those examples, or have a new example block that shows truncation? Seems like the former is the easiest and matches what we have now.

Also added @mceledonia @mcarrano @mmenestr to double check the proposed change is good. This will allow labels to be as big as they need to be based on their content, and will only truncate if the label runs out of space in the layout.

Screenshot 2023-02-27 at 7 43 35 PM

type="text"
value="{{label-editable-content--value}}"
{{else}}
currvalue="{{label-editable-content--value}}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of scope but I don't understand what this does? It's in the docs, but it isn't a valid html attr and isn't used in react. I don't think you need it - you'd just update the button text with the input value.

- Change `.pf-c-label__editable-text` back from an input to a button and set the `currvalue` of the button to the contents of the input

@@ -14,25 +14,25 @@ cssPrefix: pf-c-label-group
{{#> label-group-list-item}}
{{#> label}}
{{#> label-icon}}
<i class="fas fa-fw fa-info-circle" aria-hidden="true"></i>
<i class="fas fa-fw fa-info-circle" aria-hiddent></i>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo?

@@ -699,7 +123,7 @@ This style of label is used to add new labels to a label group.
| `.pf-c-label` | `<span>`, `<button>` | Initiates a label. Without a color modifier, the label's default style is grey. Use a color modifier to change the label color. Use a `<button>` if the label is an overflow label used in the label group. **Required** |
| `.pf-c-label__content` | `<span>`, `<a>`, `<button>` | Iniates a label content. Use as an `<a>` element if the label serves as a link. Use a `<button>` if the label serves as an action. **Required** |
| `.pf-c-label__icon` | `<span>` | Initiates a label icon. |
| `.pf-c-label__text` | `<span>` | Initiates label text. |
| `.pf-c-label__text` | `<span>` | Initiates label text. **Required** |
| `.pf-c-label__editable-text` | `<button>`, `<input>` | Initiates editable label text. See the [editable](#editable) example for details about handling behavior in Javascript.|
| `.pf-m-outline` | `.pf-c-label` | Modifies label for outline styles. |
| `.pf-m-compact` | `.pf-c-label` | Modifies label for compact styles. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to add .pf-m-truncate

@@ -410,12 +415,15 @@
@include pf-text-overflow;

max-width: var(--pf-c-label__text--MaxWidth);

&.pf-m-truncate {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it. Should this class be on .pf-c-label tho?

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattnolting @mcoker Just a couple of questions to clarify what is happening here. I'm reading this as follows. Please confirm that this is correct.

By default, labels will grow to the width of their content unless or until they are constrained by the layout or container that they live in. If there is not enough room to show all the content, that content will truncate. We're also adding a modifier to allow setting a max width of the label, in which case the content will truncate if it exceeds that length.

Do I understand this correctly? If so, I am good with this but agree that we should show this somehow in our examples.

@mattnolting mattnolting force-pushed the fix-label-4795 branch 2 times, most recently from f643f0c to baff98a Compare March 1, 2023 10:06
@mmenestr
Copy link

mmenestr commented Mar 1, 2023

I agree with @mcarrano ^ LGTM

@mcoker mcoker merged commit ac761d5 into patternfly:v5 Mar 1, 2023
@patternfly-build
Copy link

🎉 This PR is included in version 5.0.0-alpha.22 🎉

The release is available on:

Your semantic-release bot 📦🚀

mattnolting added a commit to mattnolting/patternfly that referenced this pull request Mar 2, 2023
mattnolting added a commit to mattnolting/patternfly that referenced this pull request May 18, 2023
mattnolting added a commit to mattnolting/patternfly that referenced this pull request Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug - Label - Labels break out of container with long strings of text
5 participants